Check that the path passed in to copy_file_secure_dest is a file #91
Conversation
…file This is a copy-pasta proposed fix for the issue I logged in open-power#90
| @@ -45,6 +45,14 @@ int copy_file_secure_dest(void *ctx, const char *source_file, | |||
| ssize_t r; | |||
| size_t l1; | |||
|
|
|||
| struct stat statbuf; | |||
| stat(source_file, &statbuf); | |||
There was a problem hiding this comment.
This may have a TOCTOU issue; can we fstat instead?
There was a problem hiding this comment.
I'm not a c expert... this is my best attempt to try solve the problem, using stackoverflow suggestions...
There was a problem hiding this comment.
I guess something like using fstat(source_handle, &statbuf); (maybe @ line 62) after the file has been opened sucessfully @ line 56...
(after reading the linked issue) : But maybe checking that S_ISREG() is the wrong check. What's wrong with sockets or pipes, etc... ?
If the error you want to fix is the file being a directory, just check that...
There was a problem hiding this comment.
Ack on the fstat point.
I think requiring a regular file (viua S_ISREG is reasonable here; we're parsing config files (from external filesystems), so reading from a socket/pipe is probably also a sign of something going wrong.
There was a problem hiding this comment.
Just providing a little more context here. I'd suggest something like:
commit 28d05a28b63a3413334a24620de9a9e58d539342 (HEAD -> file-dest)
Author: Brad Whittington <[email protected]>
Date: Sat Mar 5 19:05:35 2022 +0200
Check that the path passed in to copy_file_secure_dest is actually a file
This is a copy-pasta proposed fix for the issue I logged in #90
[changed to fstat(), and properly check fstat return value,
via Jeremy Kerr <[email protected]>]
Signed-off-by: Jeremy Kerr <[email protected]>
diff --git a/lib/file/file.c b/lib/file/file.c
index 6028005..5a7da0b 100644
--- a/lib/file/file.c
+++ b/lib/file/file.c
@@ -41,6 +41,7 @@ int copy_file_secure_dest(void *ctx, const char *source_file,
char template[] = "/tmp/petitbootXXXXXX";
FILE *destination_handle, *source_handle;
int destination_fd, result = 0;
+ struct stat statbuf = { 0 };
unsigned char *buffer;
ssize_t r;
size_t l1;
@@ -52,6 +53,21 @@ int copy_file_secure_dest(void *ctx, const char *source_file,
return -1;
}
+ result = fstat(fileno(source_handle), &statbuf);
+ if (result) {
+ pb_log("%s: unable to stat source file '%s': %m\n",
+ __func__, source_file);
+ fclose(source_handle);
+ return -1;
+ }
+
+ if (!S_ISREG(statbuf.st_mode)) {
+ pb_log("%s: source filename '%s' is not a regular file\n",
+ __func__, source_file);
+ fclose(source_handle);
+ return -1;
+ }
+
destination_fd = mkstemp(template);
if (destination_fd < 0) {
pb_log_fn("unable to create temp file, %m\n");If that's OK, could you include your Signed-off-by here? Or if not, feel free to make changes.
This is a copy-pasta proposed fix for the issue I logged in #90
If the path passed into
copy_file_secure_destis a valid, readable directory, it will currently proceed without error, and create a zero byte file in the destination path.